Repository navigation
Conversation
0de29a0 to
17b20fd
Compare
|
@blueorangutan package |
There was a problem hiding this comment.
Pull request overview
This PR updates CloudStack’s handling of backup repository mount options so CIFS credentials (and similar secrets like cephx keys) can be provided safely, without reintroducing command injection risk.
Changes:
- Replaces the overly restrictive
SafeCommandOptionsvalidation with a mount-options–aware validator (SafeMountCommandOptions) that allows common credential punctuation in values while keeping option keys restrictive. - Updates KVM
nasbackup.shto construct themountcommand as an argument array and pass-ooptions as a single quoted argument. - Extends
ParamProcessWorkerTestcoverage for accepted/rejected mount option patterns (credentials punctuation, base64-like values, and unsafe tokens).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
api/src/main/java/org/apache/cloudstack/api/ApiArgValidator.java |
Introduces SafeMountCommandOptions with key/value-aware parsing and tighter validation rules. |
api/src/main/java/org/apache/cloudstack/api/command/user/backup/repository/AddBackupRepositoryCmd.java |
Switches mountOptions validation to SafeMountCommandOptions. |
api/src/main/java/org/apache/cloudstack/api/command/user/backup/repository/UpdateBackupRepositoryCmd.java |
Switches mountOptions validation to SafeMountCommandOptions. |
server/src/main/java/com/cloud/api/dispatch/ParamProcessWorker.java |
Routes the new validator in parameter validation dispatch. |
server/src/test/java/com/cloud/api/dispatch/ParamProcessWorkerTest.java |
Adds tests for credential-friendly mount options and rejection of unsafe patterns. |
scripts/vm/hypervisor/kvm/nasbackup.sh |
Builds mount invocation via an array and passes options as a single argument to avoid shell splitting/expansion. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 4.20 #14009 +/- ##
============================================
+ Coverage 16.35% 16.41% +0.05%
- Complexity 13587 13656 +69
============================================
Files 5669 5669
Lines 501474 501632 +158
Branches 60913 60943 +30
============================================
+ Hits 82030 82328 +298
+ Misses 410251 410077 -174
- Partials 9193 9227 +34
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@blueorangutan package |
|
@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19037 |
abh1sar
changed the title
weizhouapache
left a comment
weizhouapache
left a comment
There was a problem hiding this comment.
@abh1sar
code lgtm
will it cause potential security issue ? I have not checked
@weizhouapache No, it shouldn't. everything the shell treats specially or expands is still rejected. @shwstppr can you please also take a look |
|
@blueorangutan package |
|
@shwstppr a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
shwstppr
left a comment
shwstppr
left a comment
There was a problem hiding this comment.
Please check if 4.20 also needs changes.
Changes look good
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19174 |
5af611f to
051d5ff
Compare
|
@blueorangutan package |
|
@abh1sar a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
Validation returned early for a blank value, so a list of nothing but whitespace was accepted although whitespace is rejected everywhere else in the list. The two sides then disagreed about it: the restore wrapper treats it as no options at all, while the backup script sees a non empty string and hands it to mount as an option of its own. An empty value still clears the options, anything else is now validated. The array holding the mount command is also declared local, as arrays assigned in a bash function are otherwise global. mount_point and dest stay as they are, the callers of mount_operation read them.
051d5ff to
8767a8d
Compare
|
@blueorangutan package |
|
@abh1sar a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19180 |
|
@blueorangutan test |
|
@weizhouapache a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian Build Failed (tid-16958) |
|
@blueorangutan test |
|
@abh1sar a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-16967)
|
5be28da to
917302d
Compare
…f stdout $? after the pipe is the status of tee, so a failed mount was treated as mounted. Read the status of mount from PIPESTATUS instead. Send the stderr of mount to the agent log rather than stdout, since the agent parses every stdout line of a stopped VM backup as a size.
917302d to
bd9b559
Compare
|
@blueorangutan package |
|
@blueorangutan test |
|
@abh1sar a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-17126)
|
|
@blueorangutan package |
|
@RosiKyu a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19682 |
There was a problem hiding this comment.
Tested the PR on KVM on Oracle Linux 9, NFS and CIFS backup repositories. The environment was deployed with 4.20.0.0 and upgraded to the PR build.
New repositories work as expected: whitespace and unsafe characters are rejected when a repository is added. Repositories that were created before the upgrade still fail, so the ticket scenario is not fixed for existing users.
1. Restore still fails for a repository created before the upgrade
Preconditions
- A NAS backup repository was added on 4.20.0.0 with a trailing space in the mount options (
vers=4.1). - A VM is assigned to an offering on that repository and has a backup.
- The management server and the KVM agents are upgraded to the PR build.
- The VM is stopped.
Steps and what each one shows
Step 1. Check the installed build and when it was installed.
Shows: the PR build was installed on 22 Oct 2026 at 16:12:59 UTC.
[root@ref-trl-12527-k-Mol9-rositsa-kyuchukova-mgmt1 ~]# rpm -q --last cloudstack-management
cloudstack-management-4.20.4.0-shapeblue22218.noarch Thu 22 Oct 2026 04:12:59 PM UTC
Step 2. Find the request that created the repository.
Shows: the repository was added on 22 Oct 2026 at 12:57:14 UTC, before the upgrade, with mountopts=vers%3D4.1%20. That is vers=4.1 followed by a space (%20).
[root@ref-trl-12527-k-Mol9-rositsa-kyuchukova-mgmt1 ~]# grep -F 'name=p14009-nfs-trail' /var/log/cloudstack/management/access.log | grep -o '^[^"]*"GET [^ ]*addBackupRepository[^ ]*'
10.0.33.194 - - [22/Oct/2026:12:57:14 +0000] "GET /client/api?address=10.0.32.4%3A%2Facs%2Fprimary%2Fref-trl-12527-k-Mol9-rositsa-kyuchukova%2Fp14009-nasbackup&apiKey=<masked>&command=addBackupRepository&mountopts=vers%3D4.1%20&name=p14009-nfs-trail&provider=nas&response=json&type=nfs&zoneid=542bd679-6186-4023-b58b-04b6957a6bcf&signature=<masked>
Step 3. Check that the repository still exists after the upgrade.
Shows: the repository is listed and looks healthy.
[root@ref-trl-12527-k-Mol9-rositsa-kyuchukova-mgmt1 ~]# cmk list backuprepositories name=p14009-nfs-trail filter=id,name,type,address
{
"backuprepository": [
{
"address": "10.0.32.4:/acs/primary/ref-trl-12527-k-Mol9-rositsa-kyuchukova/p14009-nasbackup",
"id": "f3bc71a5-4f27-4f1b-a929-3fd579ab25b0",
"name": "p14009-nfs-trail",
"type": "nfs"
}
],
"count": 1
}
Step 4. Check the VM.
Shows: the VM uses an offering on this repository and is stopped, so it can be restored.
[root@ref-trl-12527-k-Mol9-rositsa-kyuchukova-mgmt1 ~]# cmk list virtualmachines id=dc12845e-cb24-4207-8f28-348905b51b7d filter=name,instancename,state,backupofferingname
{
"count": 1,
"virtualmachine": [
{
"backupofferingname": "p14009-off-nfs-trail",
"instancename": "i-2-6-VM",
"name": "p14009-vm2",
"state": "Stopped"
}
]
}
Step 5. Restore the backup.
Shows: the restore fails with error 530.
[root@ref-trl-12527-k-Mol9-rositsa-kyuchukova-mgmt1 ~]# cmk restore backup id=fa436be9-dce4-453b-934c-5d948b707f5c
{
"account": "admin",
"accountid": "043319d2-cdfd-11f1-9e4d-1e00bd000255",
"cmd": "org.apache.cloudstack.api.command.user.backup.RestoreBackupCmd",
"completed": "2026-10-26T07:32:54+0000",
"created": "2026-10-26T07:32:54+0000",
"domainid": "af3b2b20-cdfc-11f1-9e4d-1e00bd000255",
"domainpath": "ROOT",
"jobid": "a9956a66-04b9-419e-bc10-9f3700a5d5e7",
"jobprocstatus": 0,
"jobresult": {
"errorcode": 530,
"errortext": "Error restoring VM from backup [{\"date\":\"Thu Oct 22 17:26:25 UTC 2026\",\"externalId\":\"i-2-6-VM\\/2026.10.22.17.25.27\",\"status\":\"BackedUp\",\"uuid\":\"fa436be9-dce4-453b-934c-5d948b707f5c\",\"vmId\":6}]."
},
"jobresultcode": 530,
"jobresulttype": "object",
"jobstatus": 2,
"userid": "0433eb46-cdfd-11f1-9e4d-1e00bd000255"
}
🙈 Error: async API failed for job a9956a66-04b9-419e-bc10-9f3700a5d5e7
Step 6. Check the agent log on the KVM host that ran the restore.
Shows: at the same time as the failed job (07:32:53), the agent ran mount with -o vers=4.1 (with the trailing space), and mount rejected it with "parsing error on 'vers=' option".
[root@ref-trl-12527-k-Mol9-rositsa-kyuchukova-kvm2 ~]# grep -F 'encountered the error: [mount.nfs: parsing error' /var/log/cloudstack/agent/agent.log
2026-10-26 07:32:53,861 WARN [utils.script.Script] (AgentRequest-Handler-2:[]) (logid:) Process [302147] for command [sudo /usr/bin/mount -t nfs 10.0.32.4:/acs/primary/ref-trl-12527-k-Mol9-rositsa-kyuchukova/p14009-nasbackup /usr/share/cloudstack-agent/tmp/csbackup.bcbbw10022181143658018877 -o vers=4.1 ] encountered the error: [mount.nfs: parsing error on 'vers=' option].
Expected result
The restore succeeds. This is the scenario of the ticket: restore fails when the mount options have trailing whitespace.
Actual result
The restore fails with error 530 "Error restoring VM from backup". The mount options stored before the upgrade still contain the trailing space, and mount rejects them. On 4.20 there is no API to edit the mount options of an existing repository, so the user cannot correct the value.
Same result on a CIFS repository
A CIFS repository p14009-cifs-trail was also added on 4.20.0.0 with a trailing space at the end of its mount options (username=...,password=...,vers=3.0,nobrl ). Restoring its backup on the PR build fails in the same way.
Step 7. Check the VM of the CIFS repository.
Shows: the VM is stopped, so it can be restored.
[root@ref-trl-12527-k-Mol9-rositsa-kyuchukova-mgmt1 ~]# cmk list virtualmachines name=p14009-vm4 filter=id,state
{
"count": 1,
"virtualmachine": [
{
"id": "dacdbaf5-a752-4b9e-b8d0-c74d91aac8d3",
"state": "Stopped"
}
]
}
Step 8. Restore the backup.
Shows: the restore fails with error 530.
[root@ref-trl-12527-k-Mol9-rositsa-kyuchukova-mgmt1 ~]# cmk restore backup id=144d316b-1064-4db7-adb9-3094dc4d612f
{
"account": "admin",
"accountid": "043319d2-cdfd-11f1-9e4d-1e00bd000255",
"cmd": "org.apache.cloudstack.api.command.user.backup.RestoreBackupCmd",
"completed": "2026-10-26T07:39:38+0000",
"created": "2026-10-26T07:39:37+0000",
"domainid": "af3b2b20-cdfc-11f1-9e4d-1e00bd000255",
"domainpath": "ROOT",
"jobid": "681e7196-4c9d-472e-9c15-797c9c436c45",
"jobprocstatus": 0,
"jobresult": {
"errorcode": 530,
"errortext": "Error restoring VM from backup [{\"date\":\"Thu Oct 22 13:06:09 UTC 2026\",\"externalId\":\"i-2-8-VM\\/2026.10.22.13.04.16\",\"status\":\"BackedUp\",\"uuid\":\"144d316b-1064-4db7-adb9-3094dc4d612f\",\"vmId\":8}]."
},
"jobresultcode": 530,
"jobresulttype": "object",
"jobstatus": 2,
"userid": "0433eb46-cdfd-11f1-9e4d-1e00bd000255"
}
🙈 Error: async API failed for job 681e7196-4c9d-472e-9c15-797c9c436c45
Step 9. Check the agent log on the KVM host that ran the restore.
Shows: the stored options still end with a space (nobrl ,nobrl: the space comes before the ,nobrl that is added for CIFS), and mount rejects them with "mount error(22): Invalid argument".
[root@ref-trl-12527-k-Mol9-rositsa-kyuchukova-kvm1 ~]# grep -F 'encountered the error: [mount error(22)' /var/log/cloudstack/agent/agent.log
2026-10-26 07:39:38,104 WARN [utils.script.Script] (AgentRequest-Handler-1:[]) (logid:) Process [337585] for command [sudo /usr/bin/mount -t cifs //10.0.53.204/backup /usr/share/cloudstack-agent/tmp/csbackup.rWTab12658000772677188172 -o username=root,password=<masked>,vers=3.0,nobrl ,nobrl ] encountered the error: [mount error(22): Invalid argumentRefer to the mount.cifs(8) manual page (e.g. man mount.cifs) and kernel log messages (dmesg)].
2. Backup and delete fail after the upgrade (regression)
Preconditions
The same repository and VM as in 1 (p14009-nfs-trail, p14009-vm2). On 4.20.0.0, backup from this repository worked.
Steps and what each one shows
Step 1. Take a backup of the VM.
Shows: the backup fails with error 530.
[root@ref-trl-12527-k-Mol9-rositsa-kyuchukova-mgmt1 ~]# cmk create backup virtualmachineid=dc12845e-cb24-4207-8f28-348905b51b7d
{
"account": "admin",
"accountid": "043319d2-cdfd-11f1-9e4d-1e00bd000255",
"cmd": "org.apache.cloudstack.api.command.user.backup.CreateBackupCmd",
"completed": "2026-10-26T07:37:40+0000",
"created": "2026-10-26T07:37:40+0000",
"domainid": "af3b2b20-cdfc-11f1-9e4d-1e00bd000255",
"domainpath": "ROOT",
"jobid": "a7727e6b-fceb-4ca4-bcd9-cfd06be41598",
"jobinstancetype": "Backup",
"jobprocstatus": 0,
"jobresult": {
"errorcode": 530,
"errortext": "Failed to create VM backup"
},
"jobresultcode": 530,
"jobresulttype": "object",
"jobstatus": 2,
"userid": "0433eb46-cdfd-11f1-9e4d-1e00bd000255"
}
🙈 Error: async API failed for job a7727e6b-fceb-4ca4-bcd9-cfd06be41598
Step 2. Delete an existing backup of the VM.
Shows: the delete fails with error 530.
[root@ref-trl-12527-k-Mol9-rositsa-kyuchukova-mgmt1 ~]# cmk delete backup id=fa436be9-dce4-453b-934c-5d948b707f5c
{
"account": "admin",
"accountid": "043319d2-cdfd-11f1-9e4d-1e00bd000255",
"cmd": "org.apache.cloudstack.api.command.user.backup.DeleteBackupCmd",
"completed": "2026-10-26T07:37:52+0000",
"created": "2026-10-26T07:37:52+0000",
"domainid": "af3b2b20-cdfc-11f1-9e4d-1e00bd000255",
"domainpath": "ROOT",
"jobid": "71d7513c-293a-4fe6-9c4c-2db0854ae2d8",
"jobprocstatus": 0,
"jobresult": {
"errorcode": 530,
"errortext": "Failed to delete the backup"
},
"jobresultcode": 530,
"jobresulttype": "object",
"jobstatus": 2,
"userid": "0433eb46-cdfd-11f1-9e4d-1e00bd000255"
}
🙈 Error: async API failed for job 71d7513c-293a-4fe6-9c4c-2db0854ae2d8
Step 3. Check the agent log on the KVM host.
Shows: line 55334 is the restore from section 1. For the backup (07:37:39) and the delete (07:37:52), the mount fails with the same "parsing error on 'vers=' option", and the agent answers "Failed to mount nfs store".
[root@ref-trl-12527-k-Mol9-rositsa-kyuchukova-kvm2 ~]# grep -nE 'mount.nfs: parsing error|Failed to mount nfs store' /var/log/cloudstack/agent/agent.log
55334:2026-10-26 07:32:53,861 WARN [utils.script.Script] (AgentRequest-Handler-2:[]) (logid:) Process [302147] for command [sudo /usr/bin/mount -t nfs 10.0.32.4:/acs/primary/ref-trl-12527-k-Mol9-rositsa-kyuchukova/p14009-nasbackup /usr/share/cloudstack-agent/tmp/csbackup.bcbbw10022181143658018877 -o vers=4.1 ] encountered the error: [mount.nfs: parsing error on 'vers=' option].
55986:mount.nfs: parsing error on 'vers=' option
55988:2026-10-26 07:37:39,814 DEBUG [resource.wrapper.LibvirtTakeBackupCommandWrapper] (AgentRequest-Handler-5:[]) (logid:) Failed to take VM backup: Failed to mount nfs store
55990:2026-10-26 07:37:39,815 DEBUG [cloud.agent.Agent] (AgentRequest-Handler-5:[]) (logid:) Seq 2-6594958704330689710: { Ans: , MgmtId: 32988519727701, via: 2, Ver: v1, Flags: 110, [{"org.apache.cloudstack.backup.BackupAnswer":{"result":"false","details":"Failed to mount nfs store","wait":"0","bypassHostMaintenance":"false"}}] }
55995:mount.nfs: parsing error on 'vers=' option
55997:2026-10-26 07:37:52,391 DEBUG [resource.wrapper.LibvirtDeleteBackupCommandWrapper] (AgentRequest-Handler-3:[]) (logid:) Backup delete result: Failed to mount nfs store
55999:2026-10-26 07:37:52,391 DEBUG [resource.wrapper.LibvirtDeleteBackupCommandWrapper] (AgentRequest-Handler-3:[]) (logid:) Failed to delete VM backup: Failed to mount nfs store
56001:2026-10-26 07:37:52,391 DEBUG [cloud.agent.Agent] (AgentRequest-Handler-3:[]) (logid:) Seq 2-6594958704330689711: { Ans: , MgmtId: 32988519727701, via: 2, Ver: v1, Flags: 110, [{"org.apache.cloudstack.backup.BackupAnswer":{"result":"false","details":"Failed to mount nfs store
Step 4. List the backups of the VM in the database.
Shows:
- Row 2: a backup from the same repository succeeded on 22 Oct at 13:00:29, on 4.20.0.0, before the upgrade (16:12:59). It was deleted later during testing.
- Rows 10, 11, 20 and 54: backups that failed on the PR build. Row 54 is the backup from step 1. Each failed backup leaves a row in BackingUp state.
- Row 18: the backup from step 2 is still there (it could not be deleted). It was taken on the PR build during testing, while the stored value was temporarily changed to
vers=4.1without the space. The value was then set back tovers=4.1.
[root@ref-trl-12527-k-Mol9-rositsa-kyuchukova-mgmt1 ~]# mysql -uroot -p cloud -t -e "select id,uuid,status,external_id,date,removed from backups where vm_id=(select id from vm_instance where
> uuid='dc12845e-cb24-4207-8f28-348905b51b7d') order by id;"
Enter password:
+----+--------------------------------------+-----------+------------------------------+---------------------+---------------------+
| id | uuid | status | external_id | date | removed |
+----+--------------------------------------+-----------+------------------------------+---------------------+---------------------+
| 2 | 31834206-11f3-4005-bb4c-6210ced0424a | BackedUp | i-2-6-VM/2026.10.22.12.59.38 | 2026-10-22 13:00:29 | 2026-10-22 17:26:28 |
| 10 | 5d2effd7-ce14-41bb-a2e1-78cc1ef6bb11 | BackingUp | i-2-6-VM/2026.10.22.17.14.31 | 2026-10-22 17:14:31 | 2026-10-22 17:14:31 |
| 11 | 9af56945-b574-49cd-89eb-8caca78b17f7 | BackingUp | i-2-6-VM/2026.10.22.17.17.01 | 2026-10-22 17:17:01 | 2026-10-22 17:17:01 |
| 18 | fa436be9-dce4-453b-934c-5d948b707f5c | BackedUp | i-2-6-VM/2026.10.22.17.25.27 | 2026-10-22 17:26:25 | NULL |
| 20 | 2495dff4-67be-4bdd-a6e4-8d9f5f2150f4 | BackingUp | i-2-6-VM/2026.10.22.17.36.06 | 2026-10-22 17:36:07 | 2026-10-22 17:36:07 |
| 54 | d7ef169c-8902-4a26-841d-046ba34fbf33 | BackingUp | i-2-6-VM/2026.10.26.07.37.39 | 2026-10-26 07:37:40 | 2026-10-26 07:37:40 |
+----+--------------------------------------+-----------+------------------------------+---------------------+---------------------+
Expected result
Backup and delete keep working after the upgrade, as they did on 4.20.0.0 with the same repository.
Actual result
Both fail with error 530 ("Failed to create VM backup", "Failed to delete the backup"), because mount rejects the stored options with "parsing error on 'vers=' option". Before the upgrade, backups from this repository succeeded, so the PR breaks backup and delete for existing repositories. Scheduled backups stop, and old backups cannot be deleted.
Description
The SafeCommandOptions whitelist added for command injection hardening in 56ad044 only accepts [A-Za-z0-9,._=:/+-] and whitespace.
Mount options are how credentials reach a CIFS backup repository, and any realistic value is rejected: an Active Directory username such as user@domain, or a password containing @ ! # % ^ ~. Adding or updating such a repository fails with "contains unsupported or unsafe characters".
The list is now parsed for what it is, a comma separated list of "key" or "key=value" entries, with the punctuation that appears in credentials allowed in values only. It is not a loosening across the board. Keys keep the old restrictive character set, and everything the shell treats specially or expands is still rejected in both: whitespace, quotes, $ ` ; | & < > ( ) { } [ ] \ and the glob characters * and ?. Whitespace was previously accepted and is not any more, since that is what would let a value turn into extra mount arguments.
nasbackup.sh interpolated the options into the mount command unquoted, so a value containing whitespace or a glob was split or expanded by the shell before mount saw it. The command is now built as an array and the options passed as a single quoted argument, so the option list cannot influence anything but the -o argument regardless of what validation allows through.
Not that this doesn't affect existing repositories, only the new ones.
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?